hooks: add pre-access blocking hook#1077
Conversation
sberthier
force-pushed
the
feature/pre-access-hook
branch
from
bad9e4e to
2467d64
Compare
Acconut
left a comment
Acconut
left a comment
There was a problem hiding this comment.
Wow, thank you very much for this huge PR! Congratulations on adding a new hook on your own. I can a look through it focusing on the implementation (and less on the tests and documentation) for now, and left a few comments. But overall this is looking great!
|
|
||
| // All files info that will be access by http request | ||
| // Use an array because of Upload-Concat that may target several files | ||
| Files []FileInfo |
There was a problem hiding this comment.
Can we call this Uploads, similar to the Upload field in HookEvent itself?
| Files []FileInfo | ||
| } | ||
|
|
||
| func newHookEvent(c *httpContext, info *FileInfo, accessInfo *AccessInfo) HookEvent { |
There was a problem hiding this comment.
I would prefer to split the access-related logic into a new function
func newHookEvent(c *httpContext, mode AccessMode, uploads []FileInfo) HookEvent {
event := newHookEvent(c, nil)
event.Access = {
Mode: mode,
Uploads: uploads,
}
return event
}while keeping access out of newHookEvent:
func newHookEvent(c *httpContext, info *FileInfo) HookEvent {
Then we should also be able to remove newAccessInfo.
What do you think?
| if handler.config.PreUploadCreateCallback != nil { | ||
| resp2, changes, err := handler.config.PreUploadCreateCallback(newHookEvent(c, info)) | ||
| access := newAccessInfo(AccessModeRead, partialFileInfos) | ||
| resp2, changes, err := handler.config.PreUploadCreateCallback(newHookEvent(c, &info, &access)) |
There was a problem hiding this comment.
For concatenation, I think we should first emit a pre-access hook to check the permissions for read access on the partial uploads. If that succeeds we can continue with the upload creation and the corresponding pre-create hook. Then users don't have to duplicate access-related logic in pre-access and pre-create.
A good location for that hook is likely a few lines above after the call to handler.sizeOfUploads.
| return HookEvent{ | ||
| Context: c, | ||
| Upload: info, | ||
| Upload: upload, |
There was a problem hiding this comment.
In hindsight, it would be great if we could set Upload: nil, but that would a require a change to the type, which would be breaking.
to prepare pre-access hook
- on Head/Get/Patch/Delete request - use new AccessInfo in HookEvent with AccessMode and FileInfo list - use RejectAccess in response to return 403 by default if rejected
when Upload-Concat is used, to check authorization for instance
- add some README.md to guide usage - fix grpc example Makefile proto path
sberthier
force-pushed
the
feature/pre-access-hook
branch
from
2467d64 to
9433698
Compare
|
I made the changes you suggested, except the last one to set |
|
Thank you for the updates, I will look into this in two weeks and then we should be able to get this merged. |
|
Hi everyone. Just wanted to ask, if this this is still being worked on? |
|
There hasn't been any work on this since, but we are still interested in adding this to tusd. |
|
@Acconut it seems the PR is ready to merge, any reason why is postponed please? |
|
In general, this PR looks good but I need to give it another review and probably polish a few edges. I'm not sure when I'll get to that as time is tight, but I'll try my best. Apologies for the delay! |
Any chance that this will be merged this year? This is very helpful feature. |
5 participants
pre-resumePR (hooks: add pre-resume blocking hook #1074)Access.ModeandAccess.FilestoHookEventforpre-accessandpre-create(when Upload-Concat) hook.Filesis an array because of Concatenation extension that access many files to make a new oneModeis read (Get/Head) or write (Patch/Delete)pre-accessbefore each http request Get/Head/Patch/Delete.RejectAccessin HookResponse. Http response status code will be 403, if not override in HttpResponse.close #1074